Downloader rework #481

halgari · 2023-07-25T19:37:27Z

Reworks the AdvancedHttpDownloader:

Pulls all the nested classes out into other files
Moves class specific logic into those classes, (no more Downloader.IsDownloadComplete(state), now we do state.IsComplete)
Switches to Size/Bandwidth instead of long everywhere
Starts with a HEAD request to get the file size, instead of trying to load one chunk then adjusting the entire state based on that response. This will result in one extra call per download, but TBH head requests are cheap and it drastically simplifies this code
Adds more complete tests around cancel/resume and resuming from failed network calls
No longer corrupts files when chunks early terminate due to network failure
Downloads are chunked based on size, not on number of servers, this is easy to change (about 4 lines of code) if we want some other behavior here
Inlined a lot of methods that were only used in one location

…ses they're connected with. Performs a single prefetch command for getting the file size, instead of trying to work around some chunks being the "starter" chunks. Removes a lot of methods that are used in single places and inlines them, reducing the method name bloat. Now allocates chunks based on chunk sizes, instead of total number of chunks per download.

This update includes a new method that provides a fallback mechanism for when an HTTP server does not support multipart downloads. Now, instead of throwing an exception, the application will attempt to download without resume. In addition, the local HTTP server test has been improved to better handle different HTTP methods and ensure the correct content length.

Replaced every instance of 'long' type for size and offset related variables to 'Size' type in the HttpDownloader. The 'Size' type introduces a more formalized way to represent sizes across the application, thereby enhancing code readability and maintainability. The 'Size' type also provides various helper methods and security checks that weren't available with the 'long' type. This allows for a more standardized and safer way to manipulate the respective variables across different classes and methods.

halgari · 2023-07-25T20:01:14Z

@erri120 did I break something here? Any idea why actions aren't triggering?

erri120 · 2023-07-25T20:33:26Z

@erri120 did I break something here? Any idea why actions aren't triggering?

It might be

NexusMods.App/.github/workflows/clean_environment_tests.yaml

Lines 8 to 9 in 7f168d5

    
           - "./src/**" 
        
           - "./tests/**"

. The files pushed might not match the conditions. Changing ./src to src/ might fix it.

The paths to the 'src' and 'tests' directories in the clean_environment_tests workflow have been updated. The previous paths which started with "./" were incorrect and were preventing the workflow to trigger properly. Now they are corrected to "src/**" and "tests/**" respectively.

Sewer56

General code is good, no complaints there.
Just one small/medium choice about strategy of handling local files and misc fixes to do.

The only question I have remaining is which pattern am I expected to use to pull current info about the downloader. Previously this was passed as HttpDownloaderState in IHttpDownloader.

I imagine we would want to expose an interface that makes parts of DownloadState visible.

The needed elements (at the moment) are Download Size and Amount Downloaded. First we use verbatim, second we use to calculate throughput. I don't mind doing that part since it relates to the UI code I wrote before. Just need to agree on method we're exporting data out.

tests/Networking/NexusMods.Networking.HttpDownloader.Tests/LocalHttpServer.cs

tests/Networking/NexusMods.Networking.HttpDownloader.Tests/AdvancedHttpDownloaderTests.cs

Sewer56 · 2023-07-25T20:54:39Z

src/Networking/NexusMods.Networking.HttpDownloader/SimpleHttpDownloader.cs

@@ -39,11 +39,6 @@ public async Task<Hash> DownloadAsync(IReadOnlyList<HttpRequestMessage> sources,
            // Note: If download fails, job will be reported as 'failed', and will not participate in throughput calculations.
            state.Jobs.Add(job);

-            // Integrate local download.


This and the corresponding removal in AdvancedHttpDownloader break certain tests such as the CLI test for DownloadAndInstallMod (DownloadAndInstallMod -> DownloadModFromUrl).

The problem specifically is that the systems under test will have IHttpDownloader injected into them; and the DI mappings for that interface isn't remappable on the fly. We need:

Static URL for these tests.

Test data be a mod NMA can currently recognise/install.

There's multiple possible approaches here; I have no particular preference as to which we use.
Here are some.

A. Restore the code for downloading from local file:// URIs.
This is a simple approach, and this code will only ever be used by the tests that require them; while the app gains support for a new URI scheme.

While it's not strictly part of HTTP to support this scheme, it's commonly supported in browsers and web technologies.

B. Host the data at a static location.

Either on the Nexus, the same way we have http://miami.nexus-cdn.com/100M and friends.
Or make a GitHub Release on a dummy repo and upload the files as assets; these have stable URLs.
https://github.com/{{organisation}}/{{repo}}/releases/download/{{tag}}/{{fileName}}.

C. Extend LocalHttpServer to serve files from LocalHost (this machine).

Alternative solution that requires we bootstrap the server in all tests that run on local files.
Not trivial to do in 5 minutes unless you know the APIs, but even if you don't some AI wouldn't hurt there.

I have no preference for any of the choices. Just note that if you go with B/C, or another option, also delete LocalFileDownloader as it will be dead code.

Yeah the tests are broken, so I'll fix that, I forgot I removed that code. But in general I don't like inserting overrides into classes where DI can do the trick instead.

src/Networking/NexusMods.Networking.HttpDownloader/AdvancedHttpDownloader.cs

Sewer56 · 2023-07-25T20:59:49Z

src/Networking/NexusMods.Networking.HttpDownloader/AdvancedHttpDownloader.cs

            }
        }
-
        #endregion

        #region Downloading

        private async Task DownloaderTask(DLJob job, DownloadState state, ChannelWriter<WriteOrder> writes, CancellationToken cancel)


job parameter is unused here.

src/Networking/NexusMods.Networking.HttpDownloader/AdvancedHttpDownloader.cs

src/Networking/NexusMods.Networking.HttpDownloader/DTOs/DownloadState.cs

…f enhancing code readability and consistency. Changes include: removing `public` access modifier from DownloadState class as not necessary, renaming several private fields in the AdvancedHttpDownloader class to follow underscore prefix convention for private fields, and reducing the parameters in several methods where possible to enhance clarity and readability. A comment was added in the LocalHttpServer.cs for better understanding of the function. The lack of solid changes in functionality means these refactors should not affect performance or expected results.

…roject. Preparing the HttpDownloader project for unit testing by adding InternalsVisibleTo attribute in the project file. Implementing LocalHttpServer for serving resource files during tests to simulate real-life scenarios.."

codecov · 2023-07-26T15:27:51Z

Codecov Report

Merging #481 (a56cd95) into main (5809a32) will decrease coverage by 0.05%.
Report is 1 commits behind head on main.
The diff coverage is 58.10%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #481      +/-   ##
==========================================
- Coverage   70.09%   70.05%   -0.05%     
==========================================
  Files         549      554       +5     
  Lines       14753    14802      +49     
  Branches     1036     1031       -5     
==========================================
+ Hits        10341    10369      +28     
- Misses       4140     4166      +26     
+ Partials      272      267       -5

Flag	Coverage Δ
Linux	`69.30% <58.10%> (+0.02%)`	⬆️
Windows	`69.14% <58.10%> (+<0.01%)`	⬆️
clean_environment_tests	`70.04% <58.10%> (-0.05%)`	⬇️
network_tests	`?`
networking_tests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...tworking.HttpDownloader/IHttpDownloaderSettings.cs	`37.50% <ø> (-2.50%)`	⬇️
....Networking.HttpDownloader/SimpleHttpDownloader.cs	`68.00% <ø> (-3.43%)`	⬇️
...rc/NexusMods.DataModel/Loadouts/LoadoutRegistry.cs	`76.39% <ø> (ø)`
...ttpDownloader.Tests/AdvancedHttpDownloaderTests.cs	`0.00% <0.00%> (ø)`
...ing.HttpDownloader/HttpRequestMessageExtensions.cs	`33.33% <33.33%> (ø)`
...Networking.HttpDownloader.Tests/LocalHttpServer.cs	`46.98% <35.08%> (+46.98%)`	⬆️
...sMods.Networking.HttpDownloader/DTOs/ChunkState.cs	`54.54% <54.54%> (ø)`
...s.Networking.HttpDownloader/DTOs/ServerFeatures.cs	`76.47% <76.47%> (ø)`
...NexusMods.Networking.HttpDownloader/DTOs/Source.cs	`80.00% <80.00%> (ø)`
...etworking.HttpDownloader/AdvancedHttpDownloader.cs	`79.67% <90.34%> (+6.52%)`	⬆️
... and 1 more

... and 6 files with indirect coverage changes

erri120 · 2023-08-03T12:31:27Z

This PR introduced a lot of additional logging spam:

halgari added 11 commits July 20, 2023 11:56

Document and extract the structs from the downloader code

f41b05d

I think it finally works, needs more testing though

b9d55fa

WIP

a849e48

Merge branch 'main' into downloader-rework

5a302b1

Fix resume tests

93939e1

Test cleanup

568b203

More cleanup

689315d

Final cleanup

2f49462

halgari requested a review from Sewer56 July 25, 2023 19:38

halgari added 2 commits July 25, 2023 15:04

Merge branch 'main' into downloader-rework

71d8eb2

Sewer56 requested changes Jul 25, 2023

View reviewed changes

halgari added 3 commits July 25, 2023 16:15

Add LocalHttpServer to CLI tests

a56cd95

halgari merged commit 5daa5a0 into main Jul 26, 2023
5 of 7 checks passed

halgari deleted the downloader-rework branch July 26, 2023 15:34

This was referenced Jul 27, 2023

Apparent downloader corruption with specific large files #439

Closed

StressTest "The response ended prematurely" failures #433

Closed

erri120 mentioned this pull request Aug 3, 2023

Reduce logging spam from AdvancedHttpDownloader #512

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Downloader rework #481

Downloader rework #481

halgari commented Jul 25, 2023

halgari commented Jul 25, 2023

erri120 commented Jul 25, 2023

Sewer56 left a comment

Sewer56 Jul 25, 2023

halgari Jul 25, 2023

Sewer56 Jul 25, 2023

codecov bot commented Jul 26, 2023

erri120 commented Aug 3, 2023

Downloader rework #481

Downloader rework #481

Conversation

halgari commented Jul 25, 2023

halgari commented Jul 25, 2023

erri120 commented Jul 25, 2023

Sewer56 left a comment

Choose a reason for hiding this comment

Sewer56 Jul 25, 2023

Choose a reason for hiding this comment

halgari Jul 25, 2023

Choose a reason for hiding this comment

Sewer56 Jul 25, 2023

Choose a reason for hiding this comment

codecov bot commented Jul 26, 2023

Codecov Report

erri120 commented Aug 3, 2023